Fix margin values not working for html #2977
Conversation
|
I thought about the margin/width behavior once again... the two options are basically these: The blue/red rectangle is the size and position of the canvas inside the PDF document. What you implemented is a), what I originally had in mind is b). But I'm starting to like option a) better, because it's behavior is easier to understand and makes the margins independent of the x/y and width options. What do you think? |
|
@HackbrettXXX Yeah, I think a) is better as it is more predictable and allows for more fine-tuned behavior. I feel that the functionality of b) could be possibly set in a flag or something, like |
|
Ok, so the behavior for the left side of the canvas is the same? If I specify, say, |
|
@HackbrettXXX Currently the behavior is as you specified in the issue. the content will be shifted to the left by |
|
Ok, then I suggest we change that behavior such that the margin option has no impact on the position of the canvas but clips any overlapping content on the left and on the top on the first page. |
|
@HackbrettXXX To be honest, I'm not sure how to "clip" content, will it be acceptable to simply omit the letters of text if they lay outside the margin? Instead of "clipping" the letter |
|
@HackbrettXXX Then what about margins for the second page, if we define margins as so, won't the margins cut off any content in the top part of the second page? |
|
@HackbrettXXX After some thinking on my own, I feel that margins should inset the content on their own. Your current proposal could be useful, but I feel that this should not be the default behavior. So, I would counter-propose the following:
This means making the following changes:
What do you think? |
|
After some thinking I agree with you. I think the margins and x/y properties should behave similar to HTML and CSS.
For the top/left corner HTML/CSS behaves exactly like you propose:
The dashed rect is for illustrating the padding, the blue rect would be our virtual canvas. On the bottom/right corner, in HTML/CSS the rect extends beyond the padding. This is different than your proposal. However, I still think we should implement it like you propose: cut any overlapping content at What's left to discuss would be if we call the property "margins" or "padding". I think both names would be fine. "Padding" would be closer to the HTML world, whereas "margin" intuitively fits better into jsPDF's world. What do you think? |
|
@HackbrettXXX I think margins would be better, as margins are what we usually call it when we talk about pages. Furthermore, keeping the name of the current property will be less confusing for developers using this library as well. |
|
Alright. Could you implement the changes? :) Should be basically replacing all the |
There was a problem hiding this comment.
Could you add a unit test to the test/specs/html.spec.js file with a reference PDF? See the contribution guide on how to do this.
…he right place - fix getPagesByPath (needs to respect prevPageLastElemOffset, too) - add clip path at margins for images and paths - improve doc/typings
|
I've fixed a lot of bugs and it seems like it's working quite well now. Haven't tried it with a real-world example though. I'm planning on adding a switch between two different autopaging modes:
|
|
Awesome, great work. I'm happy to try this out with a real-world example next week. Do ping me if I don't get back to you :) |
# Conflicts: # src/modules/context2d.js # src/modules/html.js # test/specs/context2d.spec.js # test/specs/html.spec.js
|
Who not want to wait for merge: clone and build https://github.com/jeffsieu/jsPDF |
|
Guys-any idea when this will be merged in? Also, wanted to confirm that the proper setting to ensure text that continues to a subsequent page w/o cutoff is autoPaging = true on the jsPDF.context2d instance. Thanks! |
|
@glebov21 your fix doesn't work
|
|
@JamesDAdams did you
@Rickardo228 did you get the chance to test it with real world examples? |
- update reference files from width/windowWidth PR (parallax#3203): there are no longer duplicate words - check in previously forgotten file
|
@HackbrettXXX I couldn't get it to work from my side, can you tell me if I did anything wrong? First, here's the code calling the const doc = await document.html(
`<div class="p-margin-zero" style="width:${marginedWidthInPixels}px;">${remarks}</div>`,
{
x,
y,
jsPDF: document,
autoPaging: 'text',
html2canvas: {
svgRendering: true,
allowTaint: true,
width: marginedWidthInPixels,
imageTimeout: 0,
backgroundColor: 'none',
scrollY: (s.page.height + s.margins.top) * pixelsPerInch,
scale: 0.01,
},
}
);What I did was:
|
|
@Yabonev you need to run |
|
@HackbrettXXX Thanks for the help, it works. Bug sample - sentence is sliced in the middle and written on two pagesHere's the "before" document page-break-test.pdf Above issue fixedHere's the pdf with up to date bundles and Margin with textHere's the pdf with paging and margin page-break-test-margin-bottom.pdf doc.html(...,
{
autoPaging: 'text',
margin: [0, 0, 1, 0], // I am using inches here, replace with what you're using.
}Margin with imagesAlso checked bottom margin for images, it works, here's the file - image-margin-bottom.pdf doc.html(...,
{
autoPaging: 'text', // also works with `'slice'` and `true`
margin: [0, 0, 1, 0],
}Margin with images and text - an issue hereHere's one with both text and image, for some reason, when an image gets slices, the text on the page, on which the image ends is white as the background, still selectable, you can check it out. text_and_image_margin.pdf Can someone else check this, don't have time for minimal repro? |
|
@Yabonev thanks for testing it. Unfortunately, I wasn't able to reproduce it by myself. This test case it("page break text + image with autoPaging: 'text'", async () => {
const text = Array.from({ length: 30 })
.map((_, i) => `ABC${i}`)
.join(" ");
const doc = jsPDF({ floatPrecision: 2, unit: "pt" });
await new Promise(resolve =>
doc.html(
`<span>${text}</span>
<img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8DwHwAFBQIAX8jx0gAAAABJRU5ErkJggg==" width="10" height="800">
<span>${text}</span>`,
{
callback: resolve,
margin: [10, 30, 10, 30],
autoPaging: "text"
}
)
);
const numberOfPages = doc.getNumberOfPages();
const pageWidth = doc.internal.pageSize.getWidth();
const pageHeight = doc.internal.pageSize.getHeight();
for (let i = 1; i <= numberOfPages; i++) {
doc.setPage(i);
doc.rect(30, 10, pageWidth - 60, pageHeight - 20);
}
comparePdf(
doc.output(),
"html-margin-text-image-page-break-text.pdf",
"html"
);
});produces this output: Could you share the markup you used for your test case? I've checked your PDF file, and it seems like there are some clip paths where they shouldn't be or the clip paths aren't properly wrapped with save/restore graphics state commands. |
|
@HackbrettXXX I somehow lost the data while testing more. Here's a case with similar input and same output, just the images are more, but smaller. Here's the image_text_test.txt file. This data is coming from rich html editor, so the image is base64 encoded string, maybe this creates the problem, but I see in your test case it's the same. Here's the result once again invisible_text_on_image_clip.pdf |
|
@Yabonev thanks. I still can't reproduce it though. When I diff my PDF with yours there are indeed differences in the save/restore graphics state commands, but I don't know why. I think I need a complete repro, otherwise I can only make guesses. Maybe we have different html2canvas versions? |
|
I think I will merge it now and create a release later. If there are still bugs, we can fix them afterwards. |
|
@HackbrettXXX Sorry for not responding, been focusing on other things, I also think that's okay. I will create a new issue with minimal reproduction when I have the time. |
|
While trying to create minimal repro, I found the issue. The text below is for those whom it may concern. Initial problem - vertical distance between
|
Update on
|
|
You're running into #3137 with the scroll_issue PDF: every letter on the second page is written in its own text chunk. This increases the file size enormously and hurts viewing performance. |
Source
jsPDF code
Output
Resolves #2924